@bkchr @ordian I think it's ready for review now. try-runtime is still failing in CI, but that'll be fixed with paritytech/try-runtime-cli#79. I've tested locally using a state from a block not containing any im-online events, and it passes OK.

@bkchr WDYT about adding offchain storage cleanup into the same PR? Seems unsound to me, as some previous offchain workers may still be there, but I'm not sure if there's any synchronization of their access to the offchain db. Could it make sense?

@bkchr WDYT about adding offchain storage cleanup into the same PR? Seems unsound to me, as some previous offchain workers may still be there, but I'm not sure if there's any synchronization of their access to the offchain db. Could it make sense?

Yeah good question, probably better to do this the next release.

@bkchr okay, I bumped the Kusama spec version in this PR to let it pass the idempotent try-runtime check, and now the only thing failing is the encointer runtime migrations that IIUC have nothing to do with this PR. So it's good to go, but I'm not sure how to sort out those encointer problems, and that CI job is required 🤷‍♂️

@bkchr okay, I bumped the Kusama spec version in this PR to let it pass the idempotent try-runtime check, and now the only thing failing is the encointer runtime migrations that IIUC have nothing to do with this PR. So it's good to go, but I'm not sure how to sort out those encointer problems, and that CI job is required 🤷‍♂️

@s0me0ne-unkn0wn Encointer is known issue, please check comment: #159 (comment)

@bkchr I think we should temporary skip check-migrations (Encointer) as required, because until it is enacted, you will need to use superpower for every PR, wdyt?

Yes, I also already thought of disabling the try runtime check for encointer. Can you please make this happen?

/merge

Enabled auto-merge in Pull Request

/merge cancel

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@s0me0ne-unkn0wn I have thought again about this pr, in the context of: paritytech/polkadot-sdk#64

I think we need to revert this PR for now or do some other manual work. The problem being that when we call candidate_events it will iterate over the events. Now, when we remove the im-online, they would be undecodable in the block that enacted the runtime upgrade. im-online is sending these events in on-initialize (aka before the parachain inherent is executed).

So, the problem could be that we miss backed candidates (not such a big issue I think). However when we miss the candidate included events we could run into a finality stall until the training wheels force approve the candidates. Given this, we should fix this.

A possible fix could be that we filter out these broken events in candidate_events before we do the actual call to the implementation.

@bkchr Hmmm 🤔
Any ideas why didn't we hit it on Rococo and Westend?
Anyway, I'll research that and will try to push on the fix to make it into the current release.

You need to be "lucky". The problem only appears when the block we enact the runtime upgrade, also contains one of these events. As they don't appear that often, triggering this bug is quite unlikely, but it can happen. At least from my understanding. I didn't looked that deep into the approval voting and if this is maybe also triggered in a different way than using the candidate events it should be fine as is.

im-online is sending these events in on-initialize (aka before the parachain inherent is executed).

Looking at that closer, the code you're pointing to is executed in on_before_session_ending, that is, not in every on_initialize but at the session boundary only. As we're scheduling a new runtime enactment at a specific block, IIUC, it wouldn't be a big problem for us to schedule it in such a way that it's guaranteed to miss the session boundary. Does that make sense?

We don't schedule the upgrade on a specific block. It depends on OpenGov voting etc when the upgrade is enacted.

We don't schedule the upgrade on a specific block

Then I probably don't fully understand what's going on here: https://polkadot.polkassembly.io/referenda/458

You got me :P I also didn't know of this :D

However, if a block number is a session change or not is still something that is not 100% predictable. Sessions end based on time. Thus, skipped blocks let a session end at an earlier block number.